Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use date instead of file mtime for updated date #3235

Merged
merged 8 commits into from Sep 21, 2019
Merged

Conversation

tomap
Copy link
Contributor

@tomap tomap commented Aug 18, 2018

Because this date is unreliable when cloning a repository from git
matches the last clone instead of lat update
This is particularly important when used in cunjunction with hexo-generator-feed

Thank you for creating a pull request to contribute to Hexo code! Before you open the request please review the following guidelines and tips to help it be more easily integrated:

  • Add test cases for the changes.
  • Passed the CI test.

Because this date is unreliable when cloning a repository from git
matches the last clone instead of lat update
This is particularly important when used in cunjunction with hexo-generator-feed
@coveralls
Copy link

coveralls commented Aug 18, 2018

Coverage Status

Coverage increased (+0.01%) to 97.157% when pulling 677f5b8 on tomap:master into 00151a1 on hexojs:master.

@tcrowe
Copy link
Contributor

tcrowe commented Sep 29, 2018

Hi @tomap. It seems like a good idea. Where does the updated variable show up in the site?

@tomap
Copy link
Contributor Author

tomap commented Oct 5, 2018

My use case is in feed plugin where the update date of a post makes no sense in my case, where I would want them to not use the file dates, but instead use the date, when no update is set.

https://github.com/hexojs/hexo-generator-feed/blob/master/atom.xml#L24

However, after giving it some thought, I think it is more an option of feed to introduce or a new post property, otherwise, it will create a change of behavior

@tcrowe
Copy link
Contributor

tcrowe commented Oct 5, 2018

@tomap Should we ask hexo core team their thoughts and see if anyone wants to talk more about this? I cannot yet understand what depends on these dates but it seems like a good idea you had.

@tomap
Copy link
Contributor Author

tomap commented Oct 6, 2018

Yes, we should, especially given the breaking change possibility

@NoahDragon
Copy link
Member

IMHO, this will change the purpose of the updated date field. I know some plugins use this field to set the updated date for the posts. Why not we modify the hexo-generator-feed to handle the date correctly, instead of changing the hexo itself?

@tomap
Copy link
Contributor Author

tomap commented Nov 3, 2018

Agreed. I'll do a PR there

@tomap tomap closed this Nov 3, 2018
@tcrowe tcrowe mentioned this pull request Dec 3, 2018
@tomap
Copy link
Contributor Author

tomap commented Jul 10, 2019

Hi, I thougth a bit more about it and it is important to have this change inside hexo.
Here is why:
1/ if I do it at the plugin level, I'll have two variables available:

  • Date (date of publication)
  • Updated, which will be either the "true" updated date (from the front-matter), and a meaningless date (mtime) but with no way for me to kwno which is which

2/ If I do it at hexo level (with an option, for backward compatibility)

  • Date (date of publication)
  • Updated, which will be either the "true" updated date (from the front-matter), or the date of publication (if no updated date is available)

So I'm reopening this PR, and will amend it soon to include the new option for backward compatility (backward compatible behavior if false or unset)

@tomap tomap reopened this Jul 10, 2019
@SukkaW
Copy link
Member

SukkaW commented Jul 11, 2019

Using file mtime as updated affect not only hexo-generator-feed but also any theme using variable post/page.updated.

For example, my hexo-theme-suka supports a feature which based on post/page.updated:

image

In my case, I believe it is a b better option to use date as updated when no updated is given in front-matter.

@curbengh
Copy link
Contributor

I prefer not dropping mtime, it's quite useful, although I personally prefer to manually set the post.updated. I'm ok with introducing new option (as shown in your latest commit.

Would updated: (in similar format as date:) in front matter resolve the issue you mentioned? Kinda similar approach as #3612.

@tomap
Copy link
Contributor Author

tomap commented Jul 30, 2019

@curbengh indeed, mentionning updated in my posts would solve the situation, but I don't want to have to mention them to fix the issue.
I want the default to work in my case
That is why I'm introducing a new option

@curbengh
Copy link
Contributor

curbengh commented Jul 30, 2019

These two seem to be conflicting:

Updated, which will be either the "true" updated date (from the front-matter), or the date of publication (if no updated date is available)
// Use post date for updated date instead of file modification date when front-matter provides no date

shouldn't it be "...when front-matter provides no updated date"?

@tomap
Copy link
Contributor Author

tomap commented Jul 30, 2019

These two seem to be conflicting:

Updated, which will be either the "true" updated date (from the front-matter), or the date of publication (if no updated date is available)
// Use post date for updated date instead of file modification date when front-matter provides no date

shouldn't it be "...when front-matter provides no updated date"?

I fixed that. Thank you

@curbengh
Copy link
Contributor

Thanks. I'll test it in the meantime.

@curbengh
Copy link
Contributor

curbengh commented Aug 6, 2019

"lib/plugins/processor/asset.js" only affects page, similar lines also need to be added to "post.js".

Copy link
Contributor

@curbengh curbengh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works fine in my local test.

@curbengh
Copy link
Contributor

curbengh commented Aug 19, 2019

Note to theme dev, If a theme currently display updated date, it needs to add another condition:

if (post.updated && post.updated.toString() !== post.date.toString()) { %>

Edit: my comment below has better option.

@tomap
Copy link
Contributor Author

tomap commented Aug 20, 2019

Or we could introduce an easier property that would leave the updated property empty in case it is not set in front matter?

@curbengh
Copy link
Contributor

curbengh commented Aug 20, 2019

The condition I mentioned is for compatibility when use_date_for_updated is enabled. Now I think about it, the following condition works better,

<% if (post.updated && !config.use_date_for_updated) { %>

Edit: Just realized the above condition will not display post.updated if use_date_for_updated is enabled. My previous comment still works.

@SukkaW SukkaW added this to the v4.0.0 milestone Aug 25, 2019
@curbengh
Copy link
Contributor

Or we could introduce an easier property that would leave the updated property empty in case it is not set in front matter?

I would prefer that option, so if (post.updated) works better.

mtime_updated perhaps?

@curbengh
Copy link
Contributor

curbengh commented Sep 18, 2019

@tomap

Or we could introduce an easier property that would leave the updated property empty in case it is not set in front matter?

Are you going to implement that option in this PR or later? Otherwise, this is good to go.

@tomap
Copy link
Contributor Author

tomap commented Sep 20, 2019

I'm not going to change this PR.

@curbengh curbengh merged commit 6f6d04e into hexojs:master Sep 21, 2019
curbengh added a commit to curbengh/hexo-starter that referenced this pull request Oct 11, 2019
- pretty_urls.trailing_index: hexojs/hexo#3691
- external_link.enable: hexojs/hexo#3675
- meta_generator: hexojs/hexo#3653
- use_date_for_updated: hexojs/hexo#3235
@curbengh curbengh mentioned this pull request Nov 28, 2019
2 tasks
yy4r pushed a commit to yy4r/blogback that referenced this pull request Jan 16, 2020
- pretty_urls.trailing_index: hexojs/hexo#3691
- external_link.enable: hexojs/hexo#3675
- meta_generator: hexojs/hexo#3653
- use_date_for_updated: hexojs/hexo#3235
@SukkaW SukkaW mentioned this pull request Apr 27, 2020
2 tasks
liyou54 pushed a commit to liyou54/blog_source that referenced this pull request Jan 19, 2023
- pretty_urls.trailing_index: hexojs/hexo#3691
- external_link.enable: hexojs/hexo#3675
- meta_generator: hexojs/hexo#3653
- use_date_for_updated: hexojs/hexo#3235
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants